-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add option to pass credentials in as http header #23
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good % maybe renaming the constant to trinoExtraCredentialHeader
(it's different from authentication credentials and may be confusing).
Can you please add a test?
We can have something similar to https://github.com/trinodb/trino-go-client/blob/master/trino/trino_test.go#L45.
@losipiuk I assume we can't write an integration test for this?
Thanks for the quick response! I'll get those changes up ASAP. |
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla. |
How long does it usually take for the CLA bot to update? I sent in a CLA as requested about 20 minutes ago. |
Got a confirmation that my CLA was accepted. Do we need to run the bot again to get the check to pass? |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvement @scbrickley. Please squash the changes into a single commit.
@losipiuk @nineinchnick Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks.
Merged as cbe5c3c (I droped |
Can we use this to pass JWT Token ? I don't see any option to authenticate with Bearer tokens |
Closes #22